Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: align 'estimatesmartfee' rpc with bitcoind #1157

Closed
wants to merge 3 commits into from

Conversation

emjshrx
Copy link
Contributor

@emjshrx emjshrx commented Jun 21, 2023

Changes the returned json attribute from 'estimatesmartfee' rpc to align with bitcoind docs . Also added a crude test to verify the same.

Fixes #1153

@theanmolsharma
Copy link
Collaborator

Thanks, good call. I'll review it this week.
While I do that if you can, please scrutinize the other RPCs as well. I know this is a tedious and boring task so no pressure.

@emjshrx
Copy link
Contributor Author

emjshrx commented Jun 25, 2023

Thanks, good call. I'll review it this week. While I do that if you can, please scrutinize the other RPCs as well. I know this is a tedious and boring task so no pressure.

Thanks @theanmolsharma . Also I noticed the coverage in RPCs are quite less so can make a test suite for them as well.

@emjshrx
Copy link
Contributor Author

emjshrx commented Jun 25, 2023

Changed the test to be in the right file and ran a formatter on the file

@theanmolsharma
Copy link
Collaborator

Changed the test to be in the right file and ran a formatter on the file

Please revert back the formatter changes.

@emjshrx emjshrx force-pushed the master branch 2 times, most recently from 1da7f24 to d1335d3 Compare June 25, 2023 07:51
@theanmolsharma
Copy link
Collaborator

Tested ACK

@pinheadmz
Copy link
Member

since this is a breaking change, we must also include a release note in the CHANGELOG.md anyone who is using this API regularly for their application will get wrecked if they upgrade and don't comply.

@emjshrx
Copy link
Contributor Author

emjshrx commented Jul 4, 2023

since this is a breaking change, we must also include a release note in the CHANGELOG.md anyone who is using this API regularly for their application will get wrecked if they upgrade and don't comply.

Thanks for bringing this to notice @pinheadmz . Have made the changes.

@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.01 🎉

Comparison is base (b005869) 69.55% compared to head (350e6b0) 69.57%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1157      +/-   ##
==========================================
+ Coverage   69.55%   69.57%   +0.01%     
==========================================
  Files         158      159       +1     
  Lines       26603    26607       +4     
==========================================
+ Hits        18505    18511       +6     
+ Misses       8098     8096       -2     
Impacted Files Coverage Δ
lib/node/rpc.js 41.96% <ø> (+0.57%) ⬆️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -425,7 +425,7 @@ describe('RPC', function() {
const {genesis} = node.network;
let entry = await node.chain.getEntry(genesis.hash.reverse());

// Get current chain tip and chain height
// Get current chain tip and chain height
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're going to touch this line, it should be like this:

      // Get current chain tip and chain height
      const chainHeight = node.chain.tip.height + 1;
      const chainTip = util.revHex(node.chain.tip.hash);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this annoying formatting change. I did not intend to touch these lines.

@pinheadmz
Copy link
Member

merged 014a104
thank you!

@pinheadmz pinheadmz closed this Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

estimatesmartfee RPC API result inconsistent with Bitcoin Core RPC API (fee vs feerate)
3 participants